feat: Regenerate libraries with protobuf-4.x - #4256
Conversation
product-auto-label
Bot
added
size: xl
Summary of ChangesHello @diegomarquezp, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a major regeneration of client libraries to incorporate protobuf-4.x, which is a foundational update impacting numerous API definitions and build processes. It also integrates new API surfaces for Google Actions SDK v2 and Google Ad Manager v1, significantly enhancing the project's scope. Concurrently, the PR refines the build and release pipeline through updated Bazel configurations, new scripts for Bazel Central Registry integration, and improved Cloud Build automation, ensuring a more robust and streamlined development workflow. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request regenerates client libraries using protobuf 4.x and introduces a substantial amount of new tooling for Bazel, GitHub Actions, and publishing to the Bazel Central Registry. My review focused on the new scripts and configuration files. I've identified a few areas for improvement in the publish-to-bcr.sh script to enhance its robustness and clarity, such as handling file writes more safely and clarifying an outdated comment. I also noted a brittle dependency in the test script publish-to-bcr_test.sh that could be improved for long-term maintainability. Additionally, there's an unused --dry_run parameter that should be addressed.
| function append_version_to_metadata() { | ||
| local version="$1" | ||
| local metadata_file="$2" | ||
| cat <<< $(jq ".versions += [\"${version}\"]" "${metadata_file}") > "${metadata_file}" |
There was a problem hiding this comment.
The use of > for in-place editing can be risky. If the jq command fails for any reason, ${metadata_file} might be truncated and left empty. A safer approach is to write the output to a temporary file first, and then replace the original file upon success.
| cat <<< $(jq ".versions += [\"${version}\"]" "${metadata_file}") > "${metadata_file}" | |
| jq ".versions += [\"${version}\"]" "${metadata_file}" > "${metadata_file}.tmp" && mv "${metadata_file}.tmp" "${metadata_file}" |
| -d|--dry_run) | ||
| dry_run="$2" | ||
| shift | ||
| ;; |
There was a problem hiding this comment.
The --dry_run option is parsed but the dry_run variable is never used in the script. This could be misleading for users expecting a dry-run functionality. If this feature is intended for future use, it might be better to add a comment indicating it's not yet implemented or remove the option until it is. If it's a remnant of a removed feature, it should be deleted.
| fi | ||
|
|
||
| if [[ -z "${templates_ref}" ]]; then | ||
| echo "assuming templates_ref to be the HEAD of master" |
There was a problem hiding this comment.
The comment mentions master, but the command uses HEAD. It would be clearer to update the comment to reflect that HEAD is being used, which typically points to the default branch (e.g., main).
| echo "assuming templates_ref to be the HEAD of master" | |
| echo "assuming templates_ref to be the HEAD of the default branch" |
| functions_to_load=$(sed '/^# parse input parameters/,$d' "$(dirname "$0")/publish-to-bcr.sh") | ||
| eval "$functions_to_load" |
There was a problem hiding this comment.
Sourcing functions from the main script using sed based on a comment is brittle. If the comment # parse input parameters is changed or removed in publish-to-bcr.sh, this test script will break. Consider refactoring the functions into a separate library file that can be sourced by both the main script and the test script for better maintainability.
1 participant
This PR regenerates the client libraries using protobuf code generator 4.x